ktor-client/ktor-client-tests/jvm/src/io/ktor/client/tests/HttpClientTest.kt (1)

210-232: Test covers exception propagation well; consider adding cancellation verification.

The test correctly validates that exceptions thrown inside the execute block are properly propagated. The withTimeout(2000) ensures the test won't hang if cancellation fails.

However, the test name suggests it verifies "ImmediatelyCancels" but only validates exception propagation. To fully verify immediate cancellation, you could add an assertion that the withTimeout didn't trigger (i.e., the exception was thrown before the 2-second timeout), or verify server-side that the connection was closed promptly.

💡 Optional: Add timing assertion to verify immediate cancellation

     `@Test`
     fun testStreamingResponseExceptionImmediatelyCancels() = runBlocking {
         val client = HttpClient(factory) {
             install(HttpTimeout) {
                 requestTimeoutMillis = HttpTimeoutConfig.INFINITE_TIMEOUT_MS
                 socketTimeoutMillis = HttpTimeoutConfig.INFINITE_TIMEOUT_MS
                 connectTimeoutMillis = HttpTimeoutConfig.INFINITE_TIMEOUT_MS
             }
         }

+        val startTime = System.currentTimeMillis()
         val exception = assertThrows<IllegalStateException> {
             withTimeout(2000) {
                 client.prepareGet("http://localhost:$serverPort/sse/delay/60000").execute { response ->
                     // Headers are received immediately
                     assertEquals(HttpStatusCode.OK, response.status)

                     // Throw an exception while waiting for the body
                     throw IllegalStateException("Test exception from execute block")
                 }
             }
         }
         assertEquals("Test exception from execute block", exception.message)
+        // Verify cancellation happened promptly, not after timeout
+        assertTrue(System.currentTimeMillis() - startTime < 1000, "Exception should propagate immediately")
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ktor-client/ktor-client-tests/jvm/src/io/ktor/client/tests/HttpClientTest.kt`
around lines 210 - 232, The test
testStreamingResponseExceptionImmediatelyCancels currently only checks exception
propagation; add a timing assertion to verify cancellation happened before the
withTimeout window by recording the start time before calling withTimeout
(wrapping the client.prepareGet(...).execute block) and assert the elapsed time
is well under 2000ms (e.g., < 1000ms) after the IllegalStateException is thrown;
reference the existing withTimeout,
client.prepareGet("http://localhost:$serverPort/sse/delay/60000") and execute
block to locate where to insert the start-time capture and elapsed-time
assertion.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

ktor-client/ktor-client-core/common/src/io/ktor/client/statement/HttpStatement.kt (1)

229-235: Preserve existing cancellation exceptions instead of always wrapping.

When cause is already a CancellationException, pass it through unchanged to keep cancellation diagnostics cleaner. Kotlin coroutines documentation explicitly recommends not wrapping existing cancellation exceptions, as this preserves the original cancellation context and avoids masking the root cause.

♻️ Proposed tweak

 internal suspend fun HttpResponse.cleanup(cause: Throwable?) {
     val job = coroutineContext.job as CompletableJob

     job.apply {
         if (cause != null) {
-            cancel(CancellationException("Exception occurred during request execution", cause))
+            val cancellation = cause as? CancellationException
+                ?: CancellationException("Exception occurred during request execution", cause)
+            cancel(cancellation)
         } else {
             complete()
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@ktor-client/ktor-client-core/common/src/io/ktor/client/statement/HttpStatement.kt`
around lines 229 - 235, In HttpResponse.cleanup, when cancelling the job
obtained via coroutineContext.job as CompletableJob, detect if the incoming
cause is already a CancellationException and pass it through unchanged (call
cancel(cause)) instead of always wrapping it in a new CancellationException;
otherwise continue wrapping with CancellationException("Exception occurred
during request execution", cause). This change should be made in the internal
suspend fun HttpResponse.cleanup(cause: Throwable?) where job.apply currently
calls cancel(...).